Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

benchmark: increase iteration number to appropriate one #50861

Closed
wants to merge 79 commits into from

Conversation

lucshi
Copy link

@lucshi lucshi commented Nov 23, 2023

Increase the iteration number from 500 to 2500 to make time consumption of crypto operation dominate the benchmark running.

Refs: #50571

Below is score improvement from 500 to 2500, measured on Cascake server.
Improvement can be a bit more if changing to 5000 but the running time will be notable longer than appropriate duration.

To make it clear, I will submit more PRs one by one to address each case in crypto, and tracked by 50571 issue.

crypto/aes-gcm-throughput.js cipher="aes-128-gcm" 554.5625838906785 n=2500 percent=144.46%
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" n=2500 percent=134.25%
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" n=2500 percent=136.22%
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" n=2500 percent=151.78%
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" n=2500 percent=125.49%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" 557.082005808399 n=2500 percent=131.03%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" n=2500 percent=131.05%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" n=2500 percent=136.30%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" n=2500 percent=149.98%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" n=2500 percent=123.60%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" 557.9763934240019 n=2500 percent=130.56%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" n=2500 percent=130.17%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" n=2500 percent=135.86%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" n=2500 percent=148.04%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" n=2500 percent=123.15%

Increase the iteration number from 500 to 2500 to make time consumption
of crypto operation dominate the benchmark running.

Fixes: nodejs#50571
@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. crypto Issues and PRs related to the crypto subsystem. labels Nov 23, 2023
Increase the iteration number from 5000 to 500000 to make time
consumption of crypto operation dominate the benchmark running.

Fixes: nodejs#50571
@H4ad H4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 26, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 26, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/50861
✔  Done loading data for nodejs/node/pull/50861
----------------------------------- PR info ------------------------------------
Title      benchmark: increase iteration number to appropriate one (#50861)
Author     Lei Shi  (@lucshi)
Branch     lucshi:my-branch -> nodejs:main
Labels     crypto, benchmark, author ready
Commits    2
 - benchmark: increase iteration number to appropriate one
 - benchmark: increase get-ciphers iteration number to appropriate one
Committers 1
 - Lei Shi 
PR-URL: https://github.com/nodejs/node/pull/50861
Refs: https://github.com/nodejs/node/issues/50571
Reviewed-By: Vinícius Lourenço Claro Cardoso 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50861
Refs: https://github.com/nodejs/node/issues/50571
Reviewed-By: Vinícius Lourenço Claro Cardoso 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 23 Nov 2023 02:51:43 GMT
   ✔  Approvals: 1
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/50861#pullrequestreview-1749348026
   ✘  This PR needs to wait 85 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/6995969572

Copy link
Member

@H4ad H4ad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before approving, can you decide if you want to include get-ciphers in this PR?

Also, can you update the first commit message to be similar to: #50698

Thanks for the PR.

@H4ad H4ad removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 26, 2023
legendecas and others added 17 commits November 27, 2023 10:25
Usages of `v8::ScriptOrModule` were removed in nodejs#44198
so the flag can be disabled by default.

PR-URL: nodejs#50616
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Otherwise, non-mutating functions such as is_tree_granted unnecessarily
require pointers to mutable data structures.

PR-URL: nodejs#50705
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#50723
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
When --node-builtin-modules-path is used, we read and create
new strings for builtins in each realm which increases the memory
usage. As a result GC may not be able to keep up with the
allocation done in the loop in the test.

As a workaround, give GC a bit more time by waiting for a timer
in the loop.

PR-URL: nodejs#50735
Refs: nodejs#50726
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#50750
Refs: nodejs#50740
Refs: nodejs/reliability#718
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#50696
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
It needs to copy the Node.js binary which is currently almost
100MB. To be safe, skip the test when the available disk space
is smaller than 120MB.

PR-URL: nodejs#50764
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Divide builtins into two lists depending on whether
they are loaded before pre-execution or at run time,
and give clearer suggestions about how to deal with them
based on the category they are in.

This helps preventing regressions like the one reported
in nodejs#45662.

PR-URL: nodejs#50708
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: nodejs#50492
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
This is a ESM benchmark, rewrite it so that we are directly
benchmarking the ESM import.meta paths and using number of
loads for op/s calculation, instead of doing it in startup
benchmarks and nesting number of process/workers spawn
for op/s calculation.

PR-URL: nodejs#50683
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#50772
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
In nodejs#50009, the return value was accidentally made part of `flush` option bullet point.

PR-URL: nodejs#50760
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
PR-URL: nodejs#50596
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Current iteration number is too small that fwrite occupies large
portion of execution time which made crypo execution time measured
inaccurate. The iteration above 1e5 makes 50% higher and stable
score.

PR-URL: nodejs#50766
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#50712
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#50712
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#50712
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
UlisesGascon and others added 27 commits November 27, 2023 10:25
PR-URL: nodejs#50486
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
The permission model's security guarantees fall apart in the presence of
relative symbolic links. When an application attempts to create a
relative symlink, the permission model currently resolves the relative
path into an absolute path based on the process's current working
directory, checks whether the process has the relevant permissions, and
then creates the symlink using the absolute target path. This behavior
is plainly incorrect for two reasons:

1. The target path should never be resolved relative to the current
   working directory. If anything, it should be resolved relative to the
   symlink's location. (Of course, there is one insane exception to this
   rule: on Windows, each process has a current working directory per
   drive, and symlinks can be created with a target path relative to the
   current working directory of a specific drive. In that case, the
   relative path will be resolved relative to the current working
   directory for the respective drive, and the symlink will be created
   on disk with the resulting absolute path. Other relative symlinks
   will be stored as-is.)
2. Silently creating an absolute symlink when the user requested a
   relative symlink is wrong. The user may (or may not) rely on the
   symlink being relative. For example, npm heavily relies on relative
   symbolic links such that node_modules directories can be moved around
   without breaking.

Because we don't know the user's intentions, we don't know if creating
an absolute symlink instead of a relative symlink is acceptable. This
patch prevents the faulty behavior by not (incorrectly) resolving
relative symlink targets when the permission model is enabled, and by
instead simply refusing the create any relative symlinks.

The fs APIs accept Uint8Array objects for paths to be able to handle
arbitrary file name charsets, however, checking whether such an object
represents a relative part in a reliable and portable manner is tricky.
Other parts of the permission model incorrectly convert such objects to
strings and then back to an Uint8Array (see 1f64147),
however, for now, this bug fix will simply throw on non-string symlink
targets when the permission model is enabled. (The permission model
already breaks existing applications in various ways, so this shouldn't
be too dramatic.)

PR-URL: nodejs#49156
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#50829
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: nodejs#49868
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#50640
Reviewed-By: Raz Luvaton <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Fixes: nodejs#50818
PR-URL: nodejs#49793
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jithil P Ponnan <[email protected]>
PR-URL: nodejs#50833
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#50844
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Original commit message:

    [flags] Remove --harmony-string-is-well-formed

    The String.prototype.isWellFormed and toWellFormed have shipped
    since M111.

    Bug: v8:13557
    Change-Id: I27e332d2fde0f9ea8ad649c016a84d2d3e0bf592
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4931269
    Reviewed-by: Shu-yu Guo <[email protected]>
    Commit-Queue: Chengzhong Wu (legendecas) <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#90398}

Refs: v8/v8@0f9ebbc
PR-URL: nodejs#50867
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#50795
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#50594
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
c-ares has made intentional changes to the behavior of TXT records
to comply with RFC 7208, which concatenates multiple strings for
the same TXT record into a single string.  Multiple TXT records
are not concatenated.

Also, response handling has changed, such that a response which is
completely invalid in formatting is thrown away as a malicious
forged/spoofed packet rather than returning EBADRESP.  This is one
step toward RFC 9018 (EDNS COOKIES) which will require the message
to at least be structurally valid to validate against spoofed
records.

Fix By: Brad House (@bradh352)

PR-URL: nodejs#50743
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fixes: nodejs#50741
Refs: nodejs#50444
PR-URL: nodejs#50816
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
- reduce copying by using std::move

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: nodejs#50846
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#50769
Reviewed-By: Matthew Aitken <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Qingyu Deng <[email protected]>
PR-URL: nodejs#50803
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#50856
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
PR-URL: nodejs#50820
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#50874
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sometimes reporters link to a repo for their repro, which
not only likely takes them more time to setup, but also is less
convenient for maintainers. Setting up a repo goes against the idea of a
minimal repro, as if it was actually minimal, they would not have
bothered creating a repo.

PR-URL: nodejs#50882
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
Every other invocation of BN_bn2binpad checks the return value. For
safety and consistency, do so in RandomPrimeTraits::EncodeOutput()
as well.

PR-URL: nodejs#50860
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#50881
Fixes: nodejs#50875
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Increase the number of iterations from 1e5 to 5e6
to avoid the test performance gap caused by inactive
V8 optimization caused by insufficient number of iterations

Refs: nodejs#50571
PR-URL: nodejs#50698
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Signed-off-by: Matteo Collina <[email protected]>
PR-URL: nodejs#50834
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruy Adorno <[email protected]>
PR-URL: nodejs#49846
Refs: nodejs/performance#106
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#49884
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@lucshi
Copy link
Author

lucshi commented Nov 27, 2023

Before approving, can you decide if you want to include get-ciphers in this PR?

Also, can you update the first commit message to be similar to: #50698

Thanks for the PR.

I will remove ciphers to make it clear. Thanks!

@lucshi lucshi closed this Nov 27, 2023
@lucshi lucshi deleted the my-branch branch November 27, 2023 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.